Skip to content

Fix phpstan/phpstan#14449: Another false positive#5438

Open
phpstan-bot wants to merge 2 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-6zjj2f4
Open

Fix phpstan/phpstan#14449: Another false positive#5438
phpstan-bot wants to merge 2 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-6zjj2f4

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

PHPStan incorrectly reported array_key_exists() as always evaluating to false when checking an array that was modified through a by-reference variable assigned from an ArrayDimFetch expression (e.g. $ref = &$arr[$id]['data']). Writes through $ref were not propagated back to $arr, so in loop iterations the type of $arr[$id]['data'] remained array{} even after keys were added via the reference.

Changes

  • src/Analyser/ExprHandler/AssignHandler.php: When processing AssignRef where the right side is an ArrayDimFetch, set up an IntertwinedVariableByReferenceWithExpr so that writes to the reference variable propagate back to the source array.
  • src/Analyser/MutatingScope.php: Added propagateRefTypeToArrayDimFetch() method that uses setOffsetValueType (replacement semantics) instead of the default assignExpression path which uses HasOffsetValueType intersection (narrowing semantics). The narrowing path fails for constant-dim ArrayDimFetch because intersecting array{data: array{}} with HasOffsetValueType('data', array{id: int}) produces never. Added hasConstantDimInChain() helper to detect when this alternative propagation is needed.
  • tests/PHPStan/Rules/Comparison/data/bug-14449.php: Regression test reproducing the original issue.
  • tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php: Test method for the regression test.

Root cause

When $resultData = &$results[$id]['data'] was processed, PHPStan set up no IntertwinedVariableByReferenceWithExpr for this case (only Variable-to-Variable references were handled). This meant that $resultData['id'] = $id only updated the type of $resultData but not $results[$id]['data']. During loop fixpoint analysis, the type of $results[$id]['data'] converged to array{} (from the explicit $results[$id]['data'] = [] assignment) without accounting for modifications through the reference. Consequently, array_key_exists('id', $results[$id]['data']) was incorrectly reported as always false.

The existing assignExpression method couldn't be used directly for propagation because specifyExpressionType uses HasOffsetValueType intersection for constant-key ArrayDimFetch, which produces never when the existing value type is incompatible with the new value type (e.g., array{} intersected with HasOffsetValueType('data', array{id: int})). The fix uses setOffsetValueType instead, which replaces the offset value type rather than narrowing it.

Test

Added tests/PHPStan/Rules/Comparison/data/bug-14449.php — reproduces the exact code from the issue: a loop with array_key_exists checks, by-reference assignment to a nested array element, and modification through the reference. The test expects no errors (the false positive is eliminated).

Fixes phpstan/phpstan#14449

…t from ArrayDimFetch

- When creating a reference from an ArrayDimFetch (e.g. $ref = &$arr['key']),
  set up IntertwinedVariableByReferenceWithExpr so writes through $ref propagate
  back to $arr
- Added propagateRefTypeToArrayDimFetch() to handle constant-dim ArrayDimFetch
  targets using setOffsetValueType instead of HasOffsetValueType intersection
  which would produce never type for incompatible constant array shapes
- New regression test in tests/PHPStan/Rules/Comparison/data/bug-14449.php
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why hasConstantDimInChain check is needed.

Also is there a simpler way to implement propagateRefTypeToArrayDimFetch ?

Replace the two-pass approach (forward pass to collect intermediate types,
backward pass to rebuild) with a single while loop that walks from leaf to
root, getting each parent's type directly from scope. Also remove unused
array_reverse import.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's a summary of what was done:

Changes made to address the review:

  1. Kept hasConstantDimInChain - The reviewer asked why it's needed. Testing confirmed it IS needed: for non-constant dims (like $list[$key]), the regular assignExpression path produces more precise type inference. The setOffsetValueType approach is only needed when constant dims are involved, because specifyExpressionType creates HasOffsetValueType intersections for constant dims that can incorrectly produce never.

  2. Simplified propagateRefTypeToArrayDimFetch - Replaced the complex two-pass approach (forward pass collecting intermediate types into arrays + array_reverse + backward pass rebuilding) with a single while loop that walks from leaf to root, getting each parent's type directly from scope via $scope->getType($expr->var). This eliminates the $dimStack, $intermediateTypes, $intermediateNativeTypes arrays and the array_reverse call. Net: -42 lines, +13 lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants